-
Notifications
You must be signed in to change notification settings - Fork 793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: alt.themes
-> alt.theme
#3618
base: main
Are you sure you want to change the base?
Conversation
Need a way to make sense of these and start GH threads: - 10 total files - 3 modules named `theme.py` - 2 modules named `__init__.py`
Was dependent on `v5` having the registry exported in `__init__`
@@ -93,7 +93,7 @@ | |||
|
|||
class AreaConfigKwds(TypedDict, total=False): | |||
""" | |||
:class:`AreaConfig` ``TypedDict`` wrapper. | |||
:class:`altair.AreaConfig` ``TypedDict`` wrapper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
altair/theme.py
Outdated
enable = themes.enable | ||
get = themes.get | ||
names = themes.names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Replaced references to `altair.theme.themes` - Tried to improve the flow, without entirely removing the term "registry"
@mattijn thanks for the quick response I'm having a hard time getting 5 working
If I've understood laurent-laporte-pro/deprecated#64 (comment) correctly, this approach doesn't work for It isn't a huge job to track (ourselves) if we've served this warning yet. |
@mattijn, as of docs: Add examples to deprecation message, the following message is displayed on first access only to Warning output
Comparing to (#3618 (comment)), the main change was removing: import altair as alt It isn't possible to trigger the warning without this import having occurred. I've got one last thing I'm experimenting with locally, that I hadn't thought of earlier. Essentially, option 4 from (#3618 (comment)) with two tweaks:
This would be in addition to the runtime warning for Especially when opting-in to Right now I'm working on getting deprecation message to pass through for this case, but still keep the versioning requirement from #3455 If we were to use this, I'd want the message to be more specific than:
|
Great! One simple question. From current: /Temp/ipykernel_9448/3158091034.py:3: AltairDeprecationWarning: Deprecated in `altair=5.5.0`. Use altair.theme instead. To wish:
|
@mattijn I would definitely prefer that, but the AFAIK, the best we could do is:
I'm open to changing the format for the part we can control: altair/altair/utils/deprecation.py Lines 23 to 32 in 0245e0f
|
That's also good👍 |
"` to all deprecation warnings #3618 (comment), #3618 (comment)
Fixed in (22d7df2) Really botched the commit message there, it should be:
|
ComparisonTo reproduce, enable the rule in diff --git a/pyproject.toml b/pyproject.toml
index 2adcd15a..33a89ae3 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -449,6 +449,7 @@ reportTypedDictNotRequiredAccess="none"
reportIncompatibleMethodOverride="none"
reportUnusedExpression="none"
reportUnsupportedDunderAll="none"
+reportDeprecated=true
include=[
"./altair/**/*.py",
"./doc/*.py",
As mentioned in (#3618 (comment)), this is opt-in. Effectively, just serving this kind of warning to those who want it. BeforeSee also #3618 (comment) After (25cbdc8) |
Open question, you mention in the deprecation warning: |
Yeah that makes sense @mattijn I hadn't noticed the convention before, but you can see it here https://docs.python.org/3/library/importlib.html#module-importlib.machinery
I'm happy for you to make the change, or I'll update it tomorrow? |
The rules here are not the same as the rules for [`LiteralString`](https://typing.readthedocs.io/en/latest/spec/literal.html#literalstring) Hoping to avoid any confusion if anyone uses this later
@mattijn I've addressed (#3618 (comment)) in (a0497e0) All of the Unresolved conversations are just to help make the review easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I approve. However, I’d like another maintainer to review this as well due to the significant impact of this PR. Once merged, it will require updates to several published packages on PyPI, such as those listed here: https://pypi.org/search/?q=altair+theme.
Thanks for the review @mattijn! Yeah that makes sense, always good to have another set of eyes |
Will close #3610, #3607
Description
This PR aims to consolidate theme related functionality behind a single namespace.
Plans & discussion have been collected in:
alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)Implementation
I would recommend reading #3618 (comment) first, as an unexpected issue arose.
Solving this contributed +1400 additional lines, of which 85% is generated code in (https://github.com/vega/altair/pull/3618/files#diff-3426a297d4278b99a32d85b11c425f5479dab750a63ba4ff77ba75870946cadf), (https://github.com/vega/altair/pull/3618/files#diff-7ed2cae75fa3376b19115773b5d58bd76f12c6e10a0ed7a03b22e098842b7b1f)
Functional Changes
alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)Tasks
alt.__init__.__getattr__
alt.theme.py
@alt.register_theme
->@alt.theme.register
alt.theme.enable
alt.theme.(active|options)
generate_schema__init__
->48e44c0
(#3618)